Upgrade flux-operator to 0.9.0 #362
Conversation
Signed-off-by: Kingdon Barrett <[email protected]>
WalkthroughThe pull request introduces several updates to the Flux Operator Helm chart, including version increments in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FluxOperator
participant Prometheus
User->>FluxOperator: Deploy Flux Operator
FluxOperator->>Prometheus: Register metrics endpoint
Prometheus->>FluxOperator: Scrape metrics
FluxOperator-->>Prometheus: Provide metrics
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml (1)
1-31: LGTM: Excellent implementation of ServiceMonitor resource.The overall implementation of the ServiceMonitor resource is well-structured, follows Helm best practices, and provides the necessary functionality for monitoring the Flux operator with Prometheus. The use of conditional creation and customizable fields allows for flexibility in deployment.
To ensure users can fully utilize this new feature:
- Update the chart's README.md to document the new ServiceMonitor functionality and its configuration options.
- Consider adding examples in the values.yaml file for common ServiceMonitor configurations to guide users in setting up monitoring for their Flux operator deployments.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
packages/system/fluxcd-operator/charts/flux-operator/values.yaml (1)
93-98: LGTM! Consider updating the schema annotation.The addition of the
serviceMonitorsection is a valuable improvement, allowing users to configure Prometheus scraping for the flux-operator. The default values are sensible, withcreate: falseensuring backwards compatibility.Consider updating the schema annotation to include the
labelsfield:# -- Prometheus Operator scraping settings. serviceMonitor: # @schema default: {"create":false,"interval":"60s","scrapeTimeout":"30s","labels":{}}packages/system/fluxcd-operator/charts/flux-operator/README.md (1)
52-52: ServiceMonitor configuration is a valuable addition.The new
serviceMonitorconfiguration option enhances the chart's monitoring capabilities by allowing integration with Prometheus Operator. This is a useful feature for users who want to monitor the flux-operator.Consider adding a brief explanation of the
serviceMonitoroption in the README, perhaps under a new "Monitoring" section. This would help users understand the purpose and benefits of this configuration.packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (1)
296-318: LGTM! Consider adding a description for thelabelsproperty.The addition of the
serviceMonitorobject is well-structured and provides a good configuration for Prometheus monitoring. The default values are sensible, and the property types are correctly defined.Consider adding a description for the
labelsproperty to clarify its purpose. For example:"labels": { + "description": "Additional labels to be added to the ServiceMonitor resource", "properties": {}, "type": "object" },This will provide more context for users configuring the chart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (2 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/README.md (2 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml (8 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/values.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (15)
packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (3)
21-22: Maintainer information has been updated.The individual maintainers have been replaced with a team entry. This change might indicate a shift in the project's maintenance structure.
Let's verify if this change is consistent with other project metadata:
#!/bin/bash # Description: Check for consistency of new maintainer information # Test: Search for old maintainer information rg --type yaml --type markdown "stefan\.prodan@control-plane\.io|soule\.ba@control-plane\.io" # Test: Search for new maintainer information rg --type yaml --type markdown "flux-enterprise@control-plane\.io|ControlPlane Flux Team"
28-28: Chart version updated to match app version.The chart version has been updated from 0.6.0 to 0.9.0, aligning with the app version update. This is a good practice for version consistency.
Let's ensure this version update is reflected in the project's documentation:
#!/bin/bash # Description: Check for version update in documentation # Test: Search for new version in README and other markdown files rg --type markdown "0\.9\.0"
11-11: Significant version update detected.The app version has been updated from v0.6.0 to v0.9.0. This is a substantial version jump that likely includes significant changes or new features in the Flux Operator.
To ensure consistency, let's verify if this version update is reflected in other relevant files:
✅ Verification successful
App version updated consistently across the codebase.
The
appVersionhas been successfully updated tov0.9.0inChart.yamland is consistently reflected inREADME.mdand the relevantvalidate.yamlfiles.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency of version 0.9.0 across relevant files # Test: Search for version references in other files rg --type yaml --type markdown "0\.9\.0" --glob '!Chart.yaml'Length of output: 483
Script:
#!/bin/bash # Description: Verify consistency of app version v0.9.0 across YAML and Markdown files, excluding Chart.yaml and shell scripts. # Search for references to v0.9.0 in YAML and Markdown files, excluding Chart.yaml and any .sh files rg --type yaml --type markdown "v0\.9\.0" --glob '!Chart.yaml' --glob '!*.sh'Length of output: 723
packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml (2)
1-3: LGTM: Conditional creation and correct resource definition.The conditional statement for creating the ServiceMonitor and the resource definition are correctly implemented. This allows for flexible deployment options and adheres to the Prometheus Operator's ServiceMonitor specification.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
4-18: LGTM: Well-structured metadata with flexible labeling and annotation options.The metadata section is well-implemented, allowing for customizable labels and annotations. The use of Helm templates for name and labels ensures consistency across resources.
Note: The yamllint warnings about indentation can be safely ignored as they are false positives due to the nature of Helm templating.
🧰 Tools
🪛 yamllint
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml (1)
22-27: LGTM! Consider using template variables for consistency.The addition of Prometheus annotations is a good improvement for enabling metrics scraping. The annotations are correctly placed and don't interfere with existing logic.
For better maintainability and consistency, consider using template variables for the port and metrics path. This would allow easier configuration changes in the future. Here's a suggested improvement:
prometheus.io/scrape: {{ .Values.metrics.enabled | default "true" | quote }} prometheus.io/port: {{ .Values.metrics.port | default "8080" | quote }} prometheus.io/path: {{ .Values.metrics.path | default "/metrics" | quote }}Then, you can define these values in your
values.yamlfile:metrics: enabled: true port: 8080 path: "/metrics"This approach would make it easier to change these values in the future if needed.
To ensure that the metrics port in the annotations matches the container port, let's verify the container spec:
✅ Verification successful
Verification Successful!
The Prometheus annotations port (8080) matches thehttp-metricscontainer port.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the metrics port in the annotations matches the container port # Test: Search for the http-metrics port in the container spec rg --type yaml 'name: http-metrics' packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yamlLength of output: 149
Script:
#!/bin/bash # Description: Extract containerPort for 'http-metrics' container rg -A3 'name: http-metrics' packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml | rg 'containerPort'Length of output: 161
packages/system/fluxcd-operator/charts/flux-operator/values.yaml (2)
100-104: LGTM! Marketplace section properly relocated.The
marketplacesection has been correctly moved to accommodate the newserviceMonitorsection. Its content remains unchanged, maintaining existing functionality.
Line range hint
1-104: Summary: ServiceMonitor configuration successfully addedThe changes to this file are focused and appropriate for the PR objective of upgrading the flux-operator. The addition of the
serviceMonitorsection enhances the chart's capabilities by allowing users to configure Prometheus scraping. The default settings are conservative (create: false), ensuring backwards compatibility while providing the option to enable this feature.The rest of the file remains unchanged, maintaining existing functionality. The relocation of the
marketplacesection is logical and doesn't impact its functionality.Overall, these changes improve the flux-operator's monitoring capabilities without introducing breaking changes.
packages/system/fluxcd-operator/charts/flux-operator/README.md (2)
3-3: Version update looks good.The version has been consistently updated to 0.9.0 across all badges, which aligns with the PR objective to upgrade flux-operator.
5-6: Repository URL update is correct.The GitHub repository URL has been updated to point directly to the flux-operator project, which is more accurate and helpful for users.
packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml (5)
338-339: LGTM. Documentation update for Condition type.The description of the Condition type has been updated to be more concise while still maintaining its informativeness. This change improves the overall documentation quality.
Also applies to: 640-641
Line range hint
1-704: Overall, the changes enhance the Flux operator's functionality and documentation.The updates to the FluxInstance CRD introduce valuable features such as tenant service account specification, resource migration control, and sharding capabilities. The documentation has also been improved. Please ensure thorough testing of these new features, particularly the sharding functionality, and consider the performance and security implications as suggested in the individual comments.
80-85: LGTM. Consider security implications of tenant service account.The addition of the
tenantDefaultServiceAccountfield enhances the multitenancy feature by allowing specification of a default service account for multitenant lockdown. The description is clear and informative.Please consider the security implications of this new field. Ensure that the specified service account has appropriate permissions and doesn't introduce any security vulnerabilities. Run the following command to check for any existing security-related comments or issues:
#!/bin/bash # Description: Check for security-related comments or issues regarding service accounts # Test: Search for security-related comments or issues rg -i "security|permission|rbac" | rg -i "service.*account|tenant"
211-217: LGTM. Consider performance implications of resource migration.The addition of the
migrateResourcesfield provides a way to control the migration of Flux custom resources to the latest API version. The default value of true ensures backward compatibility.Please consider the performance implications of this migration process, especially for large-scale deployments. It might be beneficial to add documentation or comments about potential performance impact and best practices for managing this migration. Run the following command to check for any existing performance-related comments or issues:
#!/bin/bash # Description: Check for performance-related comments or issues regarding resource migration # Test: Search for performance-related comments or issues rg -i "performance|migration|upgrade" | rg -i "resource|custom.*resource"
6-6: LGTM. Verify compatibility with the rest of the system.The update to controller-gen.kubebuilder.io/version from v0.15.0 to v0.16.1 is a minor version bump. This typically includes new features and improvements.
Please ensure that this update is compatible with the rest of the system. Run the following command to check for any breaking changes or new features that might affect your implementation:
| spec: | ||
| namespaceSelector: | ||
| matchNames: | ||
| - {{ .Release.Namespace | quote }} | ||
| selector: | ||
| matchLabels: | ||
| {{- include "flux-operator.selectorLabels" . | nindent 6 }} | ||
| endpoints: | ||
| - targetPort: 8080 | ||
| path: /metrics | ||
| interval: {{ .Values.serviceMonitor.interval }} | ||
| scrapeTimeout: {{ .Values.serviceMonitor.scrapeTimeout }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Well-configured ServiceMonitor specification with a minor suggestion.
The ServiceMonitor specification is correctly structured and uses appropriate templating for flexibility and consistency. The endpoints configuration allows for customization of scraping parameters, which is excellent.
Consider making the metrics path configurable by using a Helm value instead of hardcoding it. This would provide more flexibility for users who might want to change the metrics endpoint. For example:
path: {{ .Values.serviceMonitor.metricsPath | default "/metrics" }}Don't forget to update the values.yaml file to include this new option if you implement this suggestion.
| sharding: | ||
| description: Sharding holds the specification of the sharding configuration. | ||
| properties: | ||
| key: | ||
| default: sharding.fluxcd.io/key | ||
| description: Key is the label key used to shard the resources. | ||
| type: string | ||
| shards: | ||
| description: Shards is the list of shard names. | ||
| items: | ||
| type: string | ||
| minItems: 1 | ||
| type: array | ||
| required: | ||
| - shards | ||
| type: object |
There was a problem hiding this comment.
💡 Codebase verification
Potential Missing Tests for Sharding Feature
It appears that there are no existing tests specifically targeting the new sharding field in crds.yaml. To ensure the reliability and correctness of the sharding configuration, it's recommended to implement comprehensive tests covering various shard setups and edge cases.
- File under review:
packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml(lines 218-233)
🔗 Analysis chain
LGTM. Ensure thorough testing of the new sharding feature.
The addition of the sharding field introduces a powerful sharding capability to the Flux operator. The key field allows customization of the label key used for sharding, while the required shards field ensures a complete sharding configuration.
This feature could significantly impact how resources are distributed and managed. Please ensure thorough testing of the sharding feature, including edge cases and various shard configurations. Consider adding integration tests specifically for this feature. Run the following command to check for any existing tests related to sharding:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing tests related to sharding
# Test: Search for test files or test cases related to sharding
rg -i "test|spec" | rg -i "shard|partition"
Length of output: 33661
|
Great bot! There is a corresponding update that I'll put in a separate PR: around flux-operator v0.7.0 the I haven't tested this change (only manual deploys of flux-operator 0.9.0) - there should not be breaking changes, I expect it will be a drop-in replacement. The flux-instance chart is way more flexible than what we built for our system chart. |
Builds on #362 The main issue we will have to solve (maybe with a patch) is that `cluster.domain` is always specified in this chart; I'm reading to try to recall how we solved this last time. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Updated the Flux Operator Helm chart to version 0.9.0, introducing enhanced configuration options for service monitoring and resource management. - Added a new `ServiceMonitor` resource for Prometheus integration. - Introduced a `serviceMonitor` configuration option with default values for scraping settings. - New `FluxInstance` resource configuration file added for deploying a Flux instance. - **Documentation** - Updated README files to reflect new version and provide installation instructions for the Flux instance. - Added a `NOTES.txt` file directing users to Flux CD operator documentation. - **Bug Fixes** - Corrected links in documentation and ensured proper metadata for the new chart. - **Chores** - Restructured configuration files for improved organization and clarity. - Introduced a `.helmignore` file to streamline package building. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Kingdon Barrett <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]> Co-authored-by: Andrei Kvapil <[email protected]>
Summary by CodeRabbit
New Features
ServiceMonitorresource for Prometheus metrics scraping.serviceMonitor, including scrape interval and timeout settings.Bug Fixes
Documentation
serviceMonitorsettings.Chores